[keybindings] Aligned keybindings with VSCode #5326

JPinkney · 2019-05-31T15:30:51Z

This PR changes keybindings to align themselves with how VSCode keybinding policy works. In VSCode, keybindings always register even if there is a conflict and the one that is last registered (and has the highest priority wins i.e. scoping). For example, if I have a keybinding that opens the terminal and that same keybinding also creates a new file, when I press that keybinding the new file will be created because it was registered last.

It also makes it so that you are able to see all keybindings registered to a command, instead of just one and implements removal rules.

Signed-off-by: Josh Pinkney joshpinkney@gmail.com

JPinkney · 2019-05-31T15:44:22Z

@marechal-p @akosyakov Do you guys mind reviewing when you get a chance as well since you were both involved in the convo on spectrum about this 😄

akosyakov · 2019-06-01T04:46:00Z

@JPinkney please reference fixed issue in the commit message

packages/core/src/browser/keybinding.ts

packages/plugin-ext/src/main/browser/keybindings/keybindings-contribution-handler.ts

packages/core/src/browser/keybinding.ts

akosyakov · 2019-06-17T12:42:39Z

packages/core/src/browser/keybinding.ts

            this.keymaps[scope].push(binding);
+            this.keybindingsChanged.fire(undefined);


a bit on sure about it, on each fire menus and keybinding widget gets rerendered, it would be better to fire once for the whole keymap and fire once after all keybindings contributions are loaded (don't fire before at all)

@marechal-p @kittaakos could you help to check whether it would affect electron perf startup

I did some testing:

for initial load everything is good, since no menus is rendered before all initial keybindins are loaded

for consequent keybindings though for each binding registration top-level menus are rerendered

akosyakov

I've tested registration of duplicate and usage of duplicate keybindings + reverting them. Generally it works good, except excessive events on each change. Depending on what clients are doing it can be not so harmful, e.g. rerendering browser menus is cheap, but not sure about an electron or updating the keybinding widget with some search.

akosyakov · 2019-06-19T08:15:58Z

I'm merging it if noone objets by the evening. If there are perf issues with electron we resolve them as follow-ups.

kittaakos · 2019-06-19T12:39:39Z

I am giving it a try now...

kittaakos

Please rebase from the master first.

kittaakos · 2019-06-19T12:52:19Z

I could not verify the performance as it is still using the old code which had the menu issues. Please rebase from the theia:master. Thank you!

kittaakos · 2019-06-19T13:04:00Z

@JPinkney, please ping me if you have updated the PR and I try it again as soon as I can.

JPinkney · 2019-06-19T14:01:40Z

@kittaakos I've rebased on master!

kittaakos · 2019-06-19T15:21:32Z

Thank you, @JPinkney. I have verified the performance only, 👍 I did not check the code.

akosyakov · 2019-07-01T07:33:34Z

@JPinkney please rebase and let's merge it

akosyakov · 2019-07-02T08:58:09Z

I'm rebasing and merging it.

vince-fugnitto · 2019-07-02T13:03:04Z

Originally #5621 (comment)

It seems to work correctly, but the reset code in the keybindings-widget needs to be adjusted.
It looks like when the keybinding resets (goes back to its default value), the removal rules are not removed from the keymaps.json.

Example:

update a command's keybinding.

[
    {
        "command": "core.close.tab",
        "keybinding": "alt+a",
        "context": ""
    },
    {
        "command": "-core.close.tab",
        "keybinding": "alt+w",
        "context": ""
    }
]

Perform a reset (the following remains in the keymaps.json).

[
    {
        "command": "-core.close.tab",
        "keybinding": "alt+w",
        "context": ""
    }
]

This means that I will no longer be able to use the keybinding alt+w for Close Tab since it is part of the removal rule.

akosyakov · 2019-07-02T13:04:06Z

@vince-fugnitto please take care of reviewing it after @JPinkney addresses the issue

vince-fugnitto · 2019-07-02T13:04:37Z

@vince-fugnitto please take care of reviewing it after @JPinkney addresses the issue

Sure! :)

JPinkney · 2019-07-08T12:19:55Z

@vince-fugnitto I've fixed the issue with the reset!

vince-fugnitto · 2019-07-08T12:26:06Z

@vince-fugnitto I've fixed the issue with the reset!

Great! I'll try it out :)

vince-fugnitto

I tested the keybindings-widget revert functionality, it all works correctly now thank you!
@akosyakov the comment I had about the revert has been addressed.

packages/keymaps/src/browser/keybindings-widget.tsx

akosyakov

I've reviewed code. It looks good except one place can be simplified.

akosyakov · 2019-07-09T07:11:18Z

@tsmaeder can you nominate @JPinkney as a Theia committer please that he can merge, open CQs and so on as well

Signed-off-by: Josh Pinkney <joshpinkney@gmail.com>

JPinkney · 2019-07-16T17:59:53Z

@akosyakov I don't think @tsmaeder has nominated me but I've fixed up your comment and it should be ready for merge

This reverts commit 1f9472e. Reverts #5326. Closes #5749. Signed-off-by: Akos Kitta <kittaakos@typefox.io>

JPinkney requested review from benoitf and evidolob as code owners May 31, 2019 15:30

akosyakov requested review from vince-fugnitto and paul-marechal June 1, 2019 04:45

akosyakov reviewed Jun 1, 2019

View reviewed changes

packages/core/src/browser/keybinding.ts Outdated Show resolved Hide resolved

akosyakov reviewed Jun 1, 2019

View reviewed changes

packages/core/src/browser/keybinding.ts Outdated Show resolved Hide resolved

akosyakov reviewed Jun 1, 2019

View reviewed changes

packages/plugin-ext/src/main/browser/keybindings/keybindings-contribution-handler.ts Show resolved Hide resolved

JPinkney force-pushed the vscode-keys3 branch from 5b0c4d4 to ffa13dd Compare June 3, 2019 13:22

JPinkney mentioned this pull request Jun 4, 2019

Eclipse Keybindings Extensions has no Effect #5113

Open

JPinkney mentioned this pull request Jun 11, 2019

Remove monaco prefix from commands #5422

Closed

akosyakov mentioned this pull request Jun 17, 2019

PR diff does not always recognize PR gitpod-io/gitpod#618

Closed

akosyakov reviewed Jun 17, 2019

View reviewed changes

akosyakov approved these changes Jun 17, 2019

View reviewed changes

kittaakos self-requested a review June 19, 2019 12:48

kittaakos suggested changes Jun 19, 2019

View reviewed changes

JPinkney force-pushed the vscode-keys3 branch 2 times, most recently from a249873 to 5ce046e Compare June 19, 2019 13:57

kittaakos approved these changes Jun 19, 2019

View reviewed changes

akosyakov mentioned this pull request Jul 2, 2019

Aligned keybindings with VSCode #5621

Closed

akosyakov added keybindings issues related to keybindings vscode issues related to VSCode compatibility labels Jul 5, 2019

JPinkney force-pushed the vscode-keys3 branch 2 times, most recently from 9d50407 to 2b74fd1 Compare July 8, 2019 12:17

vince-fugnitto approved these changes Jul 8, 2019

View reviewed changes

akosyakov reviewed Jul 9, 2019

View reviewed changes

packages/keymaps/src/browser/keybindings-widget.tsx Outdated Show resolved Hide resolved

akosyakov approved these changes Jul 9, 2019

View reviewed changes

Fixed eclipse-theia#5303. Aligned keybindings with VSCode

16e7ca4

Signed-off-by: Josh Pinkney <joshpinkney@gmail.com>

JPinkney force-pushed the vscode-keys3 branch from 2b74fd1 to 16e7ca4 Compare July 16, 2019 17:59

akosyakov merged commit 1f9472e into eclipse-theia:master Jul 16, 2019

502647092 mentioned this pull request Jul 19, 2019

[Terminal] SomeKey Can't Work #5749

Closed

kittaakos mentioned this pull request Jul 22, 2019

Revert "[keybindings] Aligned keybindings with VSCode" #5770

Merged

kittaakos added a commit that referenced this pull request Jul 22, 2019

Revert "Fixed #5303. Aligned keybindings with VSCode"

5b153fa

This reverts commit 1f9472e. Reverts #5326. Closes #5749. Signed-off-by: Akos Kitta <kittaakos@typefox.io>

kittaakos pushed a commit that referenced this pull request Jul 23, 2019

Revert "Fixed #5303. Aligned keybindings with VSCode"

dd51244

This reverts commit 1f9472e. Reverts #5326. Closes #5749. Signed-off-by: Akos Kitta <kittaakos@typefox.io>

kittaakos pushed a commit that referenced this pull request Jul 23, 2019

Revert "Fixed #5303. Aligned keybindings with VSCode"

5137616

This reverts commit 1f9472e. Reverts #5326. Closes #5749. Signed-off-by: Akos Kitta <kittaakos@typefox.io>

akosyakov mentioned this pull request May 19, 2020

respect keybinding scope and registration order during evaluation #7839

Merged

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[keybindings] Aligned keybindings with VSCode #5326

[keybindings] Aligned keybindings with VSCode #5326

JPinkney commented May 31, 2019

JPinkney commented May 31, 2019

akosyakov commented Jun 1, 2019

akosyakov Jun 17, 2019

akosyakov Jun 17, 2019

akosyakov Jun 17, 2019

akosyakov left a comment •

edited

Loading

akosyakov commented Jun 19, 2019

kittaakos commented Jun 19, 2019 •

edited

Loading

kittaakos left a comment

kittaakos commented Jun 19, 2019

kittaakos commented Jun 19, 2019

JPinkney commented Jun 19, 2019

kittaakos commented Jun 19, 2019

akosyakov commented Jul 1, 2019

akosyakov commented Jul 2, 2019

vince-fugnitto commented Jul 2, 2019

akosyakov commented Jul 2, 2019

vince-fugnitto commented Jul 2, 2019

JPinkney commented Jul 8, 2019

vince-fugnitto commented Jul 8, 2019

vince-fugnitto left a comment

akosyakov left a comment

akosyakov commented Jul 9, 2019

JPinkney commented Jul 16, 2019

		this.keymaps[scope].push(binding);
		this.keybindingsChanged.fire(undefined);

[keybindings] Aligned keybindings with VSCode #5326

[keybindings] Aligned keybindings with VSCode #5326

Conversation

JPinkney commented May 31, 2019

JPinkney commented May 31, 2019

akosyakov commented Jun 1, 2019

akosyakov Jun 17, 2019

Choose a reason for hiding this comment

akosyakov Jun 17, 2019

Choose a reason for hiding this comment

akosyakov Jun 17, 2019

Choose a reason for hiding this comment

akosyakov left a comment • edited Loading

Choose a reason for hiding this comment

akosyakov commented Jun 19, 2019

kittaakos commented Jun 19, 2019 • edited Loading

kittaakos left a comment

Choose a reason for hiding this comment

kittaakos commented Jun 19, 2019

kittaakos commented Jun 19, 2019

JPinkney commented Jun 19, 2019

kittaakos commented Jun 19, 2019

akosyakov commented Jul 1, 2019

akosyakov commented Jul 2, 2019

vince-fugnitto commented Jul 2, 2019

akosyakov commented Jul 2, 2019

vince-fugnitto commented Jul 2, 2019

JPinkney commented Jul 8, 2019

vince-fugnitto commented Jul 8, 2019

vince-fugnitto left a comment

Choose a reason for hiding this comment

akosyakov left a comment

Choose a reason for hiding this comment

akosyakov commented Jul 9, 2019

JPinkney commented Jul 16, 2019

akosyakov left a comment •

edited

Loading

kittaakos commented Jun 19, 2019 •

edited

Loading